Skip to content

Conversation

@tejasbadadare
Copy link
Contributor

@tejasbadadare tejasbadadare commented Apr 17, 2025

Summary

  1. Add the ability to mark subscriptions as permanent. Permanent subscriptions disallow destructive actions, such as removing price feeds or withdrawing funds.
  2. Allow anyone to fund a subscription, not just the manager.
  3. Refactor tests with helper methods since they were getting a bit unwieldy.

Rationale

  1. It's valuable for some consumers to lock down a subscription so that it can't be tampered by a manager. They may simply be using a vault manager hot wallet there, not a multisig. That wallet could potentially rug the subscription. But, other users may find it useful to be able to stop a subscription, e.g. someone subscribes to a memecoin that goes to 0 and they want to stop the sub. Thus, we add an optional isPermanent flag to the subscription, which will disallow destructive actions when enabled. Destructive actions are defined as:

    1. Removing price feeds from a subscription
    2. Withdrawing funds from a subscription
    3. Disabling the isPermanent flag
  2. No reason to lock down addFunds. Perhaps the community wants to sponsor a feed. It's also operationally simpler for a customer to use any wallet to fund.

How has this been tested?

  • Current tests cover my changes
  • Added new tests
  • Manually tested the code

@vercel
Copy link

vercel bot commented Apr 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Apr 21, 2025 7:06pm
component-library ⬜️ Ignored (Inspect) Visit Preview Apr 21, 2025 7:06pm
entropy-debugger ⬜️ Ignored (Inspect) Visit Preview Apr 21, 2025 7:06pm
insights ⬜️ Ignored (Inspect) Visit Preview Apr 21, 2025 7:06pm
proposals ⬜️ Ignored (Inspect) Visit Preview Apr 21, 2025 7:06pm
staking ⬜️ Ignored (Inspect) Visit Preview Apr 21, 2025 7:06pm

revert IllegalPermanentSubscriptionModification();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can also effectively disable a subscription by (1) changing the readerWhitelist or (2) making the gas config so limiting that no tx ever lands on chain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think you could also do it by changing the updateCriteria

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good points -- will disallow changing the whitelist or the config. truly "permanent."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realized they can also just set isActive to false 🤦 updated

Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approved this but then realized there are some issues here

Comment on lines 114 to 116
for (uint i = 0; i < currentParams.priceIds.length; i++) {
bool found = false;
for (uint j = 0; j < newParams.priceIds.length; j++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, we're stuck doing an m x n linear search in Solidity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cant create in-memory hashmaps so yeah, back to the basics 🫠

@tejasbadadare tejasbadadare enabled auto-merge (squash) April 21, 2025 19:36
currentParams.gasConfig.maxPriorityFeeMultiplierCapPct
) {
revert IllegalPermanentSubscriptionModification();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these gas parameters could cause some problems still. The chain usage will change over time, and these parameters may need to be tweaked. If you can't change them, then they could initially be set too low, and then the chain gets more congested and then you don't get updates.

Maybe we need to set these to infinity for the permanent subscriptions? not sure exactly what to do.

(this is a problem we can resolve later also, so I'm approving this PR)

@tejasbadadare tejasbadadare merged commit ca1200a into main Apr 21, 2025
10 checks passed
@tejasbadadare tejasbadadare deleted the tb/pulse-scheduler/permanent-subscriptions branch April 21, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants